[flutter_local_notifications] Refactor the README into platform-specific guides #2477
Conversation
|
Just finished the Android page, I will try to use a similar style for the other platforms. |
|
Sorry for the slow reply - busy month. I will take a look @Levi-Lesches |
|
@MaikuB The branch is at the halfway point: It has the setup guides for all three platforms that require it (android, iOS, and Windows) that explains all the compile-time steps and platform code changes needed to get their apps compiling. I also finished the cross-platform usage guide, which describes at a high level how the plugin is used and all the common features between the platforms (basically, all the methods on Now all that's left is to make a platform-specific usage guide for all the platforms (android, darwin, linux, windows, and web), with details on all the initialization settings, permissions nuances, and notification details. These are mentioned throughout the docs I wrote, so it's important I get them ready soon, but this is also a good time to start a review if you want to since the docs that are pushed are complete thorough. Once I've finished all the other guides, I'll rewrite the main README to be shorter and more to-the-point, with a table of links to the other setup guides. Also, can you confirm that MacOS and Linux don't have any special build / platform folder setup, like |
|
@MaikuB Ready for a review! Check out https://github.com/Levi-Lesches/flutter_local_notifications/tree/readme-refactor/flutter_local_notifications and all the links in the table there (1,300 lines of docs...) |
|
The tests can probably be fixed by just running them again. I made a PR to fix the windows tests at #2696 |
MaikuB
left a comment
MaikuB
left a comment
There was a problem hiding this comment.
Thanks a lot for this. Hopefully I've reviewed everything and left comments where appropriate. In addition to those comments, these are some general callouts
General comments
- Whilst I didn't put a style guide for this, be mindful of use of capitalisation. What I had been doing is to use sentence casing as much as possible even for titles. I can see titles had remained in sentence casing. However, the links are in title case though the application hasn't been consistent. I haven't commented on the each instance on where sentence casing should be used. Let me know if you'd like me to do so
- Review usages of comma vs colon when listing out properties/methods alongside their purposes. I've commented on most of these instances that can be reviewed
- A lot of paragraphs are being repeated in each of the platform-specific usage guides. There a way to reduce the repetition? One of the goals was to help the docs easier to digest but the repetition can add more "noise" for readers to sift through as they go through each platform
- Look at breaking up long running sentences. I've commented on some instances of this
onprefix is used for callbacks that indicate a particular event took place and shouldn't be used for methods. By using theonprefix for method names this breaks the rule of methods being named after verbs. Method names should also summarise what happens within the method as well. Furthermore, will not semantically make sense either to keep theonprefix. An example of addressing this issue is to use
onDidReceiveNotificationResponse: processNotificationResponse
This will translate to an English sentence along the lines of "on receiving a notification response, process the notification response". As opposed to
onDidReceiveNotificationResponse: onNotificationTap
This will translate to an English sentence along the lines of "on receiving a notification response, on tapping the notification".
sidenote: I had also noticed inconsistent naming (tap vs press) and past/present tense (tap vs tapped). Addressing what I mentioned will solve this and keep it simple. I recommend keeping this on the back of your mind for your own development projects. It's better to stick to one (e.g. tap vs press) and avoid using synonyms that can lead to confusion in a team environment. This may also lead to questions around if there are rules when one is used over the other
| > Given how quickly the Flutter ecosystem evolves, the minimum Flutter SDK version will be bumped occasionally to make it easier to maintain the plugin. Note that the official plugins already follow a similar approach. If this affects your applications (e.g., you need to support an older OS version), you may need to consider maintaining your own fork. | ||
| > [!IMPORTANT] | ||
| > | ||
| > **Local Notifications** are very different than **Push Notifications**! Push notifications, as offered by Firebase Cloud Messaging and Apple's Push Notifications Service, are sent from the server to your users directly, while local notifications are generated by the app on the user's device and not from your server. |
There was a problem hiding this comment.
| > **Local Notifications** are very different than **Push Notifications**! Push notifications, as offered by Firebase Cloud Messaging and Apple's Push Notifications Service, are sent from the server to your users directly, while local notifications are generated by the app on the user's device and not from your server. | |
| > **Local notifications** are very different than **push notifications**! Push notifications, as offered by Firebase Cloud Messaging (FCM) and Apple Push Notification Service (APNs), are sent from the server to your users directly. Local notifications are generated by the app on the user's device and not from your server. |
Changes proposed there to
- address casing
- fix naming of what APNs represents
- include abbreviations for FCM and APNs and
- avoid long sentences
|
|
||
| - `description`: will be shown to the user in the settings app, but not in the notifications themselves | ||
| - `groupId`: which notification channel group this channel belongs to (see below) | ||
| - `importance`: how important notifications in this channel should be |
There was a problem hiding this comment.
I've not spotted it but it looks as though a note around the importance is missing. The documentation in the plugin use to bring attention to how a heads-up notification depended on correct importance. It has a link to this
An appropriate spot should be found to add mention of this and reference appropriate Android docs. May also be worth adding a link to this as well
|
|
||
| For example, imagine a messaging app that just got a message from another user. Your notification may include a text action, allowing the user to send a response directly within the notification. Again, the exact details here differ by platform. | ||
|
|
||
| Actions affect how your app handles responses. The foreground handler, background handler, and `getNotificationAppLaunchDetails()` method all provide you with a `NotificationResponse` object, which will contain a non-null `actionId` if an action was used, with a non-null `input` (or `data`) if a text field was used. For example, a notification can have a payload `new-message_CHAT_ID` and an `actionId` of `mark-read`. |
There was a problem hiding this comment.
See comment on foreground/background handler that has implications on this section. Once addressed and renamed, can add a link to the sections on said handlers too
|
|
||
| ## MSIX packaging | ||
|
|
||
| The [MSIX packaging format](https://learn.microsoft.com/en-us/windows/msix/overview) is a relatively new technique to package and distribute modern Windows apps. An MSIX installs an app onto the system as if it were downloaded from the Windows store (indeed, Windows store apps are packaged with MSIX). MSIX installers can also be used to update an existing application while preserving user data, making them really convenient. |
There was a problem hiding this comment.
The note around store apps being packaged as a MSIX can go out into its own sentence. There may also be opportunity to link to this or one of the subsections on packaging in the official Flutter docs
|
|
||
| ### Custom images and sounds | ||
|
|
||
| images can come from different sources: the web, MSIX bundles, files on the user's device, or Flutter assets. See the documentation for [`WindowsImage`](https://pub.dev/documentation/flutter_local_notifications/latest/flutter_local_notifications/WindowsImage-class.html) for complete details on when each time of source is usable. |
There was a problem hiding this comment.
| images can come from different sources: the web, MSIX bundles, files on the user's device, or Flutter assets. See the documentation for [`WindowsImage`](https://pub.dev/documentation/flutter_local_notifications/latest/flutter_local_notifications/WindowsImage-class.html) for complete details on when each time of source is usable. | |
| Images can come from different sources: the web, MSIX bundles, files on the user's device, or Flutter assets. See the documentation for [`WindowsImage`](https://pub.dev/documentation/flutter_local_notifications/latest/flutter_local_notifications/WindowsImage-class.html) for complete details on when each time of source is usable. |
Casing issue at the beginning
|
|
||
| ### Custom XML | ||
|
|
||
| Windows notifications are sent in the form of an XML schema, which the Windows SDK has to parse and verify. Using the `WindowsNotificationDetails` class in this library lets you skip that and build your notification with pure Dart code, but if you wanted to use raw XML for some unsupported or custom behavior, you can: |
There was a problem hiding this comment.
- Missing a bit of context towards the end by saying something along the lines of "you can call the following methods"
- The list if missing a colon
:to separate the methods and description like you have elsewhere
A quick note on this, I tried to balance the repetitiveness against the idea that each of these guides can be standalone. There's a lot of setup and configuration for each platform, so imagine a dev trying to configure their app across all major platforms. The intended flow is to first read the general usage guide, then, for each platform they intend to support, the platform-specific setup and usage guides. Someone who is setting up all the platforms might have spent days in between reading the general usage guide and the Windows-specific usage guide, so I tried to include enough in each document so that the reader can orient themselves without referring to anything else, but if they want more details, they might need to click a link. So that results in some introductory sentences being repeated, but any code examples, external references, and in-depth explanations should only be in one place. I'll give another read-through to make that more consistent. I'll also get around to tapped vs pressed vs clicked, I think I'll try to stick with There's a lot of docs, so I do think there's some value in merging sooner and making finishing touches later. There are a number of GitHub issues that pop up regularly and new users who need to set up that could really benefit from these. I personally have set up a few projects with notifications since starting this PR and have exclusively used these to breeze through everything, and I want others to benefit from that. |
These are a fair points and understand that now so can disregard my point on that. Can see how it goes based on feedback later on
"Pressed" makes sense to me and as it's agnostic. It's also used by the Flutter team e.g. onPressed
Sure. Let me know how you go with the feedback I left behind. I saw your mention elsewhere on health issues and hope you're getting better. Besides that, open-source work is typically done by those donating their spare time. If there's bits I can help then let me know and I'll see when I can make those adjustments directly to your fork. I'll also do some changes once PR is merged e.g. see if this can be used based on your changes, which had been mentioned before |
3 participants
Based on the work in #2308, this PR will split the README into platform-specific guides and topics (that can then be hosted on another website besides github, if desired), and leave the main README for Dart-specific details. Specifically, my goal is to:
Progress:
Verifying that all the new docs are correct (by going through them with a brand new app) is on my list but will take a bit longer. In the meantime, I've reviewed, revised, and re-reviewed all of these several times, and caught mistakes in the README and API docs, so I'm reasonably confident.
cc @davetrollope if you're interested
Closes #2196
Closes #2308